Skip to content

Conversation

@danieldresser-ie
Copy link
Contributor

In combination with the recent Cortex PR: ImageEngine/cortex#1499,
this implements the new way of working with shader ramp parameters that we've been discussing.

There are a lot of changes here, but most are quite trivial - it all follows pretty directly from the Cortex changes. A lot of just renaming things, though there are a few places where I was actually able to cut down on code duplication.

The commit sequence is structured so that after the first 5 commits, this will build against the new Cortex, while having made as few changes as possible ( this was handy when checking behaviour changes ).

The piece I'm most skeptical of is "FIX : ColorRamp and FloatRamp : Fix actual default value", which is why I made it a separate commit ... maybe that's getting too tricksy with the compatibility config? The advantage of this approach is that once all production files have been produced with new Gaffer, we could get rid of the compatibility config, and things would be clean, rather than needing to keep around splineUIMetadata.py for eternity.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Daniel! There's a colossal amount of change here (it pretty much brought my web browser to its knees), but I think it all makes sense. Let's chat about what we want to do with ColorSpline.osl/ColorRamp.osl compatibility in our meeting later today...

@danieldresser-ie
Copy link
Contributor Author

Currently working through the details of these comments, mostly pretty straightforward.

While looking at your comments about renaming "spline" -> "ramp" in variable names, I did some grepping for "spline" ... there are actually a bunch more places where a rename makes sense ... not going to help if your browser was struggling with the size of this PR before, but it does make sense to address them at the same time.

The only things that came up potentially needing discussion:

  • The rename of ColorSpline/FloatSpline to ColorRamp/FloatRamp: if I recall correctly, the proposal last we talked about this is that we'd go all in on compatibility shims - I'll write some tests for backwards compatibility, and as long as those work, we'll go ahead and do the rename, and change the value of the default for direction, and I guess in response to your comment, do a childAlias to rename the parameter spline to ramp as well. I suppose there is a question about how much we're actually gaining, versus just leaving the ColorSpline in place in old scenes, but if we ever want to get rid of ColorSpline, it's probably pretty worthwhile to actually do the compatibility conversions.
  • The exception currently being thrown while rendering. I guess this should be a msg instead? The reason I made it an exception is that I was trying to match the old behaviour, and previously this was an exception thrown when evaluating the ShaderAssignment. Now that the representation used in the ShaderNetwork has no problems representing connections to MonotoneCubic ramps, there's no real reason to throw while evaluating the ShaderAssignment, so the problem is caught later. Oh ... I just realized that this might require a change in the Cortex code that I was hoping would get merged today. Should IECoreScene::ShaderNetworkAlgo::convertToOSLConventions never throw? Or should we be catching whenever we call it in Gaffer? Is the expectation that connections to MonotoneCubic ramps should be ignored, and emit a warning? Before, they would halt the render, but that was because evaluating the scene would fail when it got to the ShaderAssignment.

@danieldresser-ie
Copy link
Contributor Author

I think I've now addressed all comments ... hopefully it doesn't bog your browser down too much, since you were saying this PR was already giving you issues for some reason. If it would help, I can try to pull anything that isn't a simple rename out of the big rename commit - though I'd probably do that after squashing 59f2572 into the big rename.

@danieldresser-ie
Copy link
Contributor Author

Squashed last round of fixes, added changelog entries, switched dependencies to Cortex-10.7.0.0a1.

I should mention one fix that I've squashed that wasn't in the last round of fixes to review: I must not have tested the most recent version of IECoreArnoldTest, because the last round of tests of testOSLShaders were trying to compare the addresses of shaders connected to the duplicated endpoints, and that was failing - I've switched to checking that the properties of the connected shaders are correct, and that seems to be working fine now.

This should be good for interactive testing, and as far as I know, basically ready to merge.

Linear splines in OSL only need 1 extra endpoint. This test had linear splines with 2 extra endpoints, because it started from a Cortex representation that already had an extra endpoint - that's conceptually wrong, IECore::Spline only uses duplicate end points for curve types where it is required for evaluation, not for linear.
Previously, connections to color components in OSL shaders would fail in 3Delight
IECoreScene::ShaderNetworkAlgo::expandSplineParameters was the deprecated way to handle spline parameters.

IECoreArnold::ShaderNetworkAlgo is now doing the up to date approach - there is a call to IECoreScene::ShaderNetworkAlgo::convertToOSLConventions in preprocessedNetwork, which will have already converted splines. As far as I can tell, this means that expandSplineParameters was currently not doing anything, and the fact it was still being called was simply an oversight.
@danieldresser-ie
Copy link
Contributor Author

I've removed the platform23 test. Unfortunately still got two test failures, but they seem unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending Review

Development

Successfully merging this pull request may close these issues.

3 participants